src/app/core/guard/travelperk-token.guard.ts (2)

1-9: Consider a dedicated import grouping for improved readability.

Currently, imports for Angular modules, RxJS, and application-specific services appear in a single block. It's sometimes beneficial to group imports by domain (e.g., Angular core, external libraries, internal services) to simplify scanning. This is purely optional and doesn't affect functionality.

---

32-49: Validate error status check and rethrow approach.

1. Currently, only 400 is handled as an expired connection scenario. Consider whether there are other relevant status codes (e.g., 401 or 403) that should trigger the same re-authentication flow.
2. The guard rethrows the error after handling the 400 status scenario. Make sure any upstream error handler or route error resolution is also prepared to handle it.

src/app/core/guard/travelperk-token.guard.spec.ts (2)

7-7: Remove trailing spaces.

Static analysis reports trailing spaces on this line. Removing them keeps the codebase clean and consistent with style guidelines.

7c7
-  const executeGuard: CanActivateFn = (...guardParameters) => 
+  const executeGuard: CanActivateFn = (...guardParameters) =>

🧰 Tools

🪛 eslint

[error] 7-7: Trailing spaces not allowed.

(no-trailing-spaces)

🪛 GitHub Check: lint

[failure] 7-7:
Trailing spaces not allowed

---

14-16: Add more test coverage for guard logic.

The current test only verifies that the guard is created. Consider adding tests that validate:

1. Behavior when workspace ID is missing.
2. Behavior when a 400 error is thrown by travelperkService.
3. Behavior for successful connection.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

src/app/core/guard/travelperk-token.guard.spec.ts (1)

1-1: Remove trailing spaces.

The line contains trailing spaces which should be removed.

-  
+

🧰 Tools

🪛 eslint

[error] 1-1: Trailing spaces not allowed.

(no-trailing-spaces)

🪛 GitHub Check: lint

[failure] 1-1:
Trailing spaces not allowed

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

src/app/core/services/travelperk/travelperk.service.ts (1)

33-40: Consider making the error handling more robust.

The error handling implementation is good, but could be made more resilient with a few improvements:

Consider applying these improvements:

 return this.apiService.get(`/orgs/${this.orgId}/travelperk/`, {}).pipe(
   catchError(error => {
-    if (error.status === 400 && error.error?.message?.includes('token expired')) {
+    // Define error interface for better type safety
+    interface TravelperkError {
+      status: number;
+      error?: {
+        message?: string;
+        is_expired?: boolean;
+      };
+    }
+
+    // Use constant for error message
+    const TOKEN_EXPIRED_MESSAGE = 'token expired';
+
+    // Safer error handling with type checking
+    if (error?.status === 400 && 
+        typeof error?.error?.message === 'string' && 
+        error.error.message.includes(TOKEN_EXPIRED_MESSAGE)) {
       error.error.is_expired = true;
     }
     return throwError(() => error);
   })
 );

This refactor:

1. Adds type safety with an interface
2. Makes error property checks more defensive
3. Extracts the error message to a constant

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

#!/bin/bash
# Search for usages of getTravelperkData to verify error handling
echo "Searching for getTravelperkData usage:"
rg -A 10 "getTravelperkData\(\)" --type ts

# Search for error handling patterns in components
echo "Checking error handling patterns in components:"
ast-grep --pattern 'catchError($_)' --type ts

#!/bin/bash
# Search for error handling in components that use getTravelperkData
echo "Searching for error handling in TravelPerk components:"
rg -B 5 -A 10 "error\." "src/app/integrations/travelperk/"

# Search specifically for is_expired handling
echo "Searching for is_expired handling:"
rg "is_expired" --type ts

# Check error handling in the guard
echo "Checking TravelPerk guard implementation:"
cat src/app/core/guard/travelperk-token.guard.ts

src/app/core/guard/travelperk-token.guard.ts (1)

1-9: Consider creating route constants

The navigation URL 'integrations/travelperk/onboarding/landing' is hardcoded. Consider moving it to a constants file for better maintainability.

// src/app/core/constants/routes.constant.ts
export const TRAVELPERK_ROUTES = {
  ONBOARDING_LANDING: 'integrations/travelperk/onboarding/landing'
} as const;

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

src/app/core/services/travelperk/travelperk.service.ts (2)

33-41: 🛠️ Refactor suggestion

Extract duplicated error handling logic

The error handling logic is duplicated between this method and getTravelperkTokenHealth.

Previous review suggested extracting this to a reusable operator, which is still applicable:

private handleTravelperkError = () => <T>(source: Observable<T>): Observable<T> => {
  return source.pipe(
    catchError(error => {
      if (error.status === 400) {
        if (error.error?.message?.includes('token expired') || error.error?.is_expired) {
          error.error.is_expired = true;
        }
      }
      return throwError(() => error);
    })
  );
};

---

44-44: 🛠️ Refactor suggestion

Define proper interface for token health response

Using {} as return type isn't type-safe and could lead to runtime errors.

Create and use a proper interface:

interface TravelperkTokenHealth {
  is_healthy: boolean;
  // add other relevant fields
}

🧰 Tools

🪛 Biome (1.9.4)

[error] 44-44: Don't use '{}' as a type.

Prefer explicitly define the object shape. '{}' means "any non-nullable value".

(lint/complexity/noBannedTypes)

src/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.ts (1)

485-486: Improve readability & address lint warnings

The current arrow function body can be simplified and reformatted to address lint warnings related to curly braces and spacing. Consider returning the observable directly:

- this.workspaceService.getWorkspaceGeneralSettings().pipe(catchError(error => {return of(null);})),
+ this.workspaceService.getWorkspaceGeneralSettings().pipe(
+   catchError(() => of(null))
+ ),

🧰 Tools

🪛 GitHub Check: lint

[failure] 485-485:
Statement inside of curly braces should be on next line

---

[failure] 485-485:
Requires a space after '{'

---

[failure] 485-485:
Closing curly brace should be on the same line as opening curly brace or on the line after the previous block

---

[failure] 485-485:
Requires a space before '}'

src/app/core/services/travelperk/travelperk.service.ts (1)

43-43: Review caching strategy for token health

The @Cacheable decorator is used without a cache buster. Consider:

1. Adding a cache buster for token health
2. Setting an appropriate TTL for the cache

Example implementation:

const tokenHealthCache$ = new Subject<void>();

@Cacheable({
  cacheBusterObserver: tokenHealthCache$,
  maxAge: 5 * 60 * 1000 // 5 minutes
})

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

src/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.ts (1)

485-486: Consider logging workspace settings retrieval failures

The error handling for workspace settings is good as it prevents the observable chain from breaking. However, consider logging the error for debugging purposes. Also, the code properly handles null cases by providing default values:

• employeeFieldMapping defaults to EmployeeFieldMapping.EMPLOYEE
• isImportItemsEnabled defaults to false
• auto_map_employees defaults to false

       this.workspaceService.getWorkspaceGeneralSettings().pipe(
         catchError(error => {
+          console.error('Failed to fetch workspace settings:', error);
           return of(null);
         })
       ),

Also applies to: 491-491, 499-499, 508-508

🧰 Tools

🪛 eslint

[error] 485-485: Statement inside of curly braces should be on next line.

(brace-style)

---

[error] 485-485: Requires a space after '{'.

(block-spacing)

---

[error] 485-485: Closing curly brace should be on the same line as opening curly brace or on the line after the previous block.

(brace-style)

---

[error] 485-485: Requires a space before '}'.

(block-spacing)

🪛 GitHub Check: lint

[failure] 485-485:
Statement inside of curly braces should be on next line

---

[failure] 485-485:
Requires a space after '{'

---

[failure] 485-485:
Closing curly brace should be on the same line as opening curly brace or on the line after the previous block

---

[failure] 485-485:
Requires a space before '}'

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

src/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.spec.ts (1)

518-518: Consider re-enabling this skipped test to maintain coverage and detect potential regressions.
Skipping this important test may reduce confidence in the CCC expense grouping logic.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

src/app/core/models/qbo/qbo-configuration/qbo-export-setting.model.ts (1)

261-261: Consider extracting fallback logic to a helper function for consistency.

Using a conditional access operator here ensures null safety, preventing potential runtime errors. For maintainability, you might extract the fallback logic to a helper function or a common config, making it easier to manage the default behavior across the codebase.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro